Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for commas in values for exporter.otlp.headers #6052

Closed
wants to merge 2 commits into from

Conversation

lesterhaynes
Copy link

@lesterhaynes lesterhaynes commented Dec 6, 2023

My use for the OpenTelemetry Java Agent requires that I pass headers with values containing commas to my otel collector. RFC 9110 section 5.5 has some verbiage on how to support this.

headerOneKey=headerOneValueA,HeaderOneValueB
headerTwoKey=headerTwoValue

could now be passed as

-Dotel.exporter.otlp.headers="\"headerOneKey=headerOneValueA,HeaderOneValueB\", \"headerTwoKey=headerTwoValue\""

Copy link

codecov bot commented Dec 6, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (3f1b9ed) 91.18% compared to head (719ff40) 91.18%.
Report is 13 commits behind head on main.

❗ Current head 719ff40 differs from pull request most recent head 6390294. Consider uploading reports for the commit 6390294 to get more accurate results

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #6052   +/-   ##
=========================================
  Coverage     91.18%   91.18%           
- Complexity     5623     5627    +4     
=========================================
  Files           618      618           
  Lines         16580    16586    +6     
  Branches       1642     1644    +2     
=========================================
+ Hits          15118    15124    +6     
  Misses         1013     1013           
  Partials        449      449           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lesterhaynes lesterhaynes marked this pull request as ready for review December 6, 2023 00:56
@lesterhaynes lesterhaynes requested a review from a team December 6, 2023 00:56
@lesterhaynes lesterhaynes marked this pull request as draft December 6, 2023 01:40
Copy link
Member

@jack-berg jack-berg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This behavior is governed by the spec here and I would be surprised if it was changed to support this.

A couple of thoughts:

You could use a AutoConfigurationCustomizerProvider to customizer your OTLP exporters and add the headers programatically.

Someting like:

  @Override
  public void customize(AutoConfigurationCustomizer autoConfiguration) {
    autoConfiguration.addSpanExporterCustomizer(
            (spanExporter, configProperties) -> {
              if (spanExporter instanceof OtlpGrpcSpanExporter) {
                spanExporter.shutdown().join(10, TimeUnit.SECONDS);
                return ((OtlpGrpcSpanExporter) spanExporter).toBuilder().addHeader("foo", "value,value").build();
              }
              return spanExporter;
            });
  }

There's a general moratorium on enhancing the environment variable scheme. Instead, we want to mature the file configuration scheme. There is experimental java support for file configuration in otel java as described here. The config for this type of use might look like:

# Configure tracer provider.
tracer_provider:
  processors:
    - batch:
         exporter:
           otlp:
             endpoint: ..
             headers:
               key: "value,value"

@lesterhaynes
Copy link
Author

This behavior is governed by the spec here and I would be surprised if it was changed to support this.

A couple of thoughts:

You could use a AutoConfigurationCustomizerProvider to customizer your OTLP exporters and add the headers programatically.

Someting like:

  @Override
  public void customize(AutoConfigurationCustomizer autoConfiguration) {
    autoConfiguration.addSpanExporterCustomizer(
            (spanExporter, configProperties) -> {
              if (spanExporter instanceof OtlpGrpcSpanExporter) {
                spanExporter.shutdown().join(10, TimeUnit.SECONDS);
                return ((OtlpGrpcSpanExporter) spanExporter).toBuilder().addHeader("foo", "value,value").build();
              }
              return spanExporter;
            });
  }

There's a general moratorium on enhancing the environment variable scheme. Instead, we want to mature the file configuration scheme. There is experimental java support for file configuration in otel java as described here. The config for this type of use might look like:

# Configure tracer provider.
tracer_provider:
  processors:
    - batch:
         exporter:
           otlp:
             endpoint: ..
             headers:
               key: "value,value"

Thanks for the reply! I'll look into your suggestion

@lesterhaynes
Copy link
Author

lesterhaynes commented Dec 8, 2023

Thanks for the help @jack-berg, this got me unblocked. If you don't mind, I think I may update the otel-java-instrumentation example impl with the CustomizerProvider you shared.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants